Skip to content

Android: Add failOnWarnings check to android build.gradle and configure lint as warnings as error #12040

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Android: Add failOnWarnings check to android build.gradle and configure lint as warnings as error #12040

wants to merge 10 commits into from
+19 −0

Conversation

Sangamesh1997
Copy link
Contributor

@Sangamesh1997 Sangamesh1997 commented Apr 28, 2025 •
edited
Loading

Android: Add failOnWarnings check to android build.gradle and configure lint as warnings as error.

Fixes: #6868

@Sangamesh1997 Sangamesh1997 marked this pull request as draft April 28, 2025 04:39
@Sangamesh1997 Sangamesh1997 marked this pull request as ready for review April 28, 2025 04:52
@Sangamesh1997 Sangamesh1997 marked this pull request as draft April 28, 2025 10:05
@Sangamesh1997
Copy link
Contributor Author

@vinodhabib Please review and share feedback on any changes or improvements needed.

vinodhabib
vinodhabib reviewed Apr 28, 2025
View reviewed changes
Copy link
Contributor

@vinodhabib vinodhabib left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM.

@Sangamesh1997 Sangamesh1997 marked this pull request as ready for review April 29, 2025 05:05
shivaspeaks
shivaspeaks reviewed Apr 30, 2025
View reviewed changes
Copy link
Member

@shivaspeaks shivaspeaks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've done both (linter warnings and enabling -Werror) the mentioned issues in this PR. That's fine for me. @ejona86 should be able to tell us if we shouldn't be doing like this.

android/build.gradle Outdated
lintOptions { abortOnError true }

// fail on android lint warnings if failOnWarning is set true
lintOptions {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are doing this anyway, lintOptions is deprecated. Use lint instead. (Wait for Eric's comment on this before working)

android/build.gradle Outdated
@@ -44,6 +52,14 @@ dependencies {
testImplementation libraries.truth
}

// Enforce -Werror on java compiler if failOnWarning is set
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may remove the comments as they are self explanatory.

@shivaspeaks shivaspeaks requested a review from ejona86 April 30, 2025 07:12
@ejona86 ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Apr 30, 2025
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Apr 30, 2025
ejona86
ejona86 reviewed Apr 30, 2025
View reviewed changes
@ejona86 ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label May 12, 2025
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label May 12, 2025
ejona86
ejona86 reviewed May 12, 2025
View reviewed changes
Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this fixing #6868? The issue talks about JavaCompile, but you removed that from the PR. Lint is good and helpful and we'll take it, but that was a bonus (and probably can't go in without a lot of fixes).

@Sangamesh1997
Copy link
Contributor Author

@ejona86 you're right - lint was an additional improvement, but I missed re-adding the JavaCompile configuration from the original intent of #6868. Initially I assumed that configuring tasks.withType(JavaCompile) globally already applied -Xlint:all and -Werror across all modules. I've now updated the PR to include both the lint block and the JavaCompile warnings, so all modules are covered. Please take another look, thanks!

@Sangamesh1997 Sangamesh1997 requested a review from ejona86 May 12, 2025 18:01
@ejona86 ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label May 21, 2025
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label May 21, 2025
ejona86
ejona86 reviewed May 21, 2025
View reviewed changes
Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The build should be failing because of lint warnings. I expect there will be quite a few, which is why I was letting it be done separately.

@Sangamesh1997 Sangamesh1997 requested a review from ejona86 May 27, 2025 07:51
@ejona86 ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label May 27, 2025
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label May 27, 2025
ejona86
ejona86 reviewed May 27, 2025
View reviewed changes
Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks closer, but the build failure looks like it still remains.

@kannanjgithub
Copy link
Contributor

@Sangamesh1997 For the old Android API targetted error that is causing the build failure, can you try adding the lint.xml exception mentioned here?

@ejona86
Copy link
Member

ejona86 commented Jun 3, 2025

For the old Android API targetted error that is causing the build failure, can you try adding the lint.xml exception mentioned here?

Are you thinking we'd open a new issue to fix that? That is easy to fix, so I don't know why we would silence it instead of just fixing it.

I could sort of understand if we want to silence all current lint failures so we stop introducing new ones, but really the warnings I've seen are really old so I was more concerned with fixing them, really.

@kannanjgithub
Copy link
Contributor

I assumed we really only cared about the minSdkVersion and that we didn't care about making any API changes needed for the latest Android version available. Now that I think about it I realize it is advantageous to do it rather than not do it , to be compatible with the latest platform advancements.

@ejona86
Copy link
Member

ejona86 commented Jun 3, 2025

Generally working with the new platform has been pretty easy for us; bump the number. Our users will be targeting newer versions, so if there is pain, they'd experience that pain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ejona86 ejona86 ejona86 left review comments

@shivaspeaks shivaspeaks Awaiting requested review from shivaspeaks

@vinodhabib vinodhabib Awaiting requested review from vinodhabib

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Teach Android build -PfailOnWarnings=true
6 participants
@Sangamesh1997 @kannanjgithub @ejona86 @vinodhabib @shivaspeaks @grpc-kokoro